Skip to content

Add configurable probe timeout and frequency for activator #16250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
bindrad wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add configurable probe timeout and frequency for activator #16250

bindrad wants to merge 2 commits into from

Conversation

@bindrad
Copy link

@bindrad bindrad commented Nov 20, 2025

Fixes #16249

Proposed Changes

  • Add PROBE_TIMEOUT environment variable to configure activator health check timeout (default: 300ms)
  • Add PROBE_FREQUENCY environment variable to configure activator probe frequency (default: 200ms)

Release Note

Activator probe timeout and frequency are now configurable via PROBE_TIMEOUT and PROBE_FREQUENCY environment variables.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 20, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@knative-prow
Copy link

knative-prow bot commented Nov 20, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bindrad
Once this PR has been reviewed and has the lgtm label, please assign skonto for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2025
@knative-prow
Copy link

knative-prow bot commented Nov 20, 2025

Hi @bindrad. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@knative-prow knative-prow bot requested review from dprotaso and skonto November 20, 2025 10:54
@linkvt
Copy link
Contributor

linkvt commented Nov 21, 2025

/ok-to-test

@knative-prow knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 21, 2025
@bindrad
Copy link
Author

bindrad commented Nov 22, 2025 •
edited
Loading

/test unit-tests

@bindrad
Copy link
Author

bindrad commented Nov 22, 2025

I'm not sure if istio-latest-no-mesh-tls_serving_main is failing due to my changes. Please suggest.

@linkvt
Copy link
Contributor

linkvt commented Nov 24, 2025

It's likely a flaky test indicated by a webhook deployment not being ready in the E2E test setup. Let's do a simple retest.

/retest

@linkvt
Copy link
Contributor

linkvt commented Nov 24, 2025

@bindrad Seems like the flaky test finished without errors.

I just reviewed the code and think it might make sense to only set/overwrite these values through the config-network ConfigMap instead of allowing an overwrite via environment variables. This would seem more in line with how we configure other parameters, I guess the two fields above (MaxIdleProxyConns, ...) are rather outliers.
Another idea would be a new ConfigMap config-activator, this is IMO too much overhead so I wouldn't do it.

I'm not a long-term contributor/approver though so someone else might have a better idea.

@bindrad
Copy link
Author

bindrad commented Nov 25, 2025

@linkvt Thanks for the feedback. I thought to follow the same pattern as MaxIdleProxyConns and MaxIdleProxyConnsPerHost, which are also set via environment variables. I'm fine with either approach, but I think it would be useful to make these values configurable in some form.

@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.10%. Comparing base (a94c607) to head (45e141b).
⚠️ Report is 33 commits behind head on main.

Files with missing lines Patch % Lines
pkg/activator/net/throttler.go 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #16250      +/-   ##
==========================================
+ Coverage   80.02%   80.10%   +0.07%     
==========================================
  Files         215      215              
  Lines       13327    13328       +1     
==========================================
+ Hits        10665    10676      +11     
+ Misses       2299     2293       -6     
+ Partials      363      359       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dprotaso
dprotaso reviewed Dec 4, 2025
View reviewed changes
cmd/activator/main.go
Comment on lines +87 to +88
ProbeTimeout int `split_words:"true" default:"300"`
ProbeFrequency int `split_words:"true" default:"200"`
Copy link
Member

@dprotaso dprotaso Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like more flexibility here to provide a golang duration string and parse it using https://pkg.go.dev/time#ParseDuration to prase

Copy link
Member

@dprotaso dprotaso Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise not having the unit in the name is confusing

pkg/activator/net/revision_backends.go
Comment on lines +460 to 461
probeTimeout time.Duration
probeFrequency time.Duration
Copy link
Member

@dprotaso dprotaso Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where and when is probeFrequency changed on a per revision basis?

If we're not changing things per-revision we can save ourselves all this extra code plumbing these fields down and maybe just use the constants that can be overridden by the env var

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dprotaso dprotaso dprotaso left review comments

@skonto skonto Awaiting requested review from skonto

Assignees

No one assigned

Labels

ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add configurable probe timeout for activator to support high-latency environments

3 participants

@bindrad @linkvt @dprotaso